[Core] fix gRPC handlers' unlimited active calls configuration #25626
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
mwtian
changed the title
fishbone
reviewed
Jun 9, 2022
src/ray/rpc/grpc_server.cc
Outdated
| int buffer_size = 100; | ||
| // When there is no max active RPC limit, a call will be added to the completetion | ||
| // queue before processing starts, so adding only 1 call is enough. | ||
| int buffer_size = 1; |
There was a problem hiding this comment.
I'm not very familiar with this. But does this mean we'll accept request slower than before? For example, accept the request 1by1 because the size ie only 1 and only after we do the actual processing, a new one will be added.
There was a problem hiding this comment.
The nightly tests do not show a big difference, but from reading the code of completion queue, I can see that having a number of tags buffered can make processing more efficient. Adjusted the value to be closer to the existing value.
fishbone
approved these changes
Jun 10, 2022
stephanie-wang
pushed a commit
that referenced
this pull request
Jul 5, 2022
truelegion47
pushed a commit
to truelegion47/ray
that referenced
this pull request
Jul 6, 2022
* master: (104 commits) [Serve] Java Client API and End to End Tests (ray-project#22726) [Docs] Small fix to AIR examples descriptions (ray-project#26227) [Deployment Graph] Move `Deployment` creation outside to build function (ray-project#26129) [K8s][Ray Operator] Ignore resource requests when detected container resources. (ray-project#26234) Revert "[Core] Add retry exception allowlist for user-defined filteri… (ray-project#26289) [ci] pin gpustat (ray-project#26311) [tune] fix `set_tune_experiment` (ray-project#26298) Revert "Revert "[AIR][Serve] Rename ModelWrapperDeployment -> PredictorDeployment"" (ray-project#26231) [Release] Use nightly base images for release tests (ray-project#25373) Revert "[Core] fix gRPC handlers' unlimited active calls configuration (ray-project#25626)" (ray-project#26202) [RLlib] Some Docs fixes (2). (ray-project#26265) [C++ worker] Refine worker context and more (ray-project#26281) Fix file_system_monitor.cc message (ray-project#26143) [Java] Make Java test more stable (ray-project#26282) [air] Do not warn of `checkpoint_dir` if it's coming from us (base_trainer). (ray-project#26259) [Datasets] Support drop_columns API (ray-project#26200) [Datasets] Fix max number of actors for default actor pool strategy (ray-project#26266) [ci] Stop syncer staging tests (ray-project#26273) [core][gcs] Add storage namespace to redis storage in GCS. (ray-project#25994) [workflow] Deprecate workflow.create (ray-project#26106) ...
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Why are these changes needed?
Ray's gRPC server wrapper configures a max active call setting for each handler. When the max active call is
-1, the handler is supposed to allow handling unlimited number of requests concurrently. However in practice it is often observed that handlers configured with unlimited active calls are still handling at most 100 requests concurrently.This is a result of the existing logic:
ServerCallobject (tag) before actual processing. The problem is that new ServerCalls are created on the eventloop instead of the gRPC server thread. When the event loop runs a callback from the gRPC server, the callback creates a new ServerCall object, and can run the gRPC handler to completion if the handler does not have any async step. So overall, the event loop will not run more callbacks than the initial number ofServerCalls, which is 100 in the "unlimited" mode.The solution is to create a new
ServerCallin the gRPC server thread, before sending theServerCallto the eventloop.Running some night tests to verify the fix does not introduce instabilities: https://buildkite.com/ray-project/release-tests-branch/builds/652
Also, looking into adding gRPC server / client stress tests with large number of concurrent requests.
Related issue number
Checks
scripts/format.shto lint the changes in this PR.